-
Notifications
You must be signed in to change notification settings - Fork 53
Fix "SyntaxTree is not part of the compilation" exception in orchestration analyzers #588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: YunchuWang <[email protected]>
Co-authored-by: YunchuWang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where Roslyn analyzers DURABLE0001-DURABLE0006, DURABLE0009-DURABLE0010 threw ArgumentException: SyntaxTree is not part of the compilation when analyzing orchestration methods that invoke methods from referenced projects.
Key Changes
- Modified
FindInvokedMethods()inMethodProbeOrchestrationVisitorto check if syntax trees are part of the compilation context before attempting to get their semantic models - Changed from using
semanticModel.Compilationtothis.Compilationto access the full compilation context containing all syntax trees being analyzed
| // Since the syntax tree of the callee method might be different from the caller method, we need to get the correct semantic model, | ||
| // avoiding the exception 'Syntax node is not within syntax tree'. | ||
| // We also need to check if the syntax tree is part of the compilation to avoid 'SyntaxTree is not part of the compilation' exception. | ||
| if (!this.Compilation.ContainsSyntaxTree(calleeSyntax.SyntaxTree)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this check only be necessary in the case that semanticModel.SyntaxTree != calleeSyntax.SyntaxTree? If that is the case, shouldn't the ternary assignment below be broken up into an if statement so that this check can be added only in the else case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot to check on the above comment (although some human eyes might also be good)
(edit: I guess I'm not allowed to summon the beast...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go a step further and suggest that since this check looks expensive, and analyzers are part of a more "performance critical" path, we should probably be sure to only perform it in the else case below where it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copilot is not very responsive to mentions in comment threads. you can try mention it at top level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this contain check is doing a hashlookup of syntaxtree in current set of syntax trees -O(1) perf impact is trivial . it is used in other analyzers frequently https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs#L890-L894
| // Since the syntax tree of the callee method might be different from the caller method, we need to get the correct semantic model, | ||
| // avoiding the exception 'Syntax node is not within syntax tree'. | ||
| // We also need to check if the syntax tree is part of the compilation to avoid 'SyntaxTree is not part of the compilation' exception. | ||
| if (!this.Compilation.ContainsSyntaxTree(calleeSyntax.SyntaxTree)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go a step further and suggest that since this check looks expensive, and analyzers are part of a more "performance critical" path, we should probably be sure to only perform it in the else case below where it is needed.
Analyzers threw
ArgumentException: SyntaxTree is not part of the compilationwhen analyzing orchestration methods that invoke methods from referenced projects. Affects DURABLE0001-DURABLE0006, DURABLE0009-DURABLE0010.Root cause
MethodProbeOrchestrationVisitor.FindInvokedMethods()usedsemanticModel.Compilationto get semantic models for invoked methods. When those methods exist in different syntax trees (e.g., referenced projects), the syntax tree isn't part of that compilation instance.Changes
In
OrchestrationAnalyzer.cs, modifiedFindInvokedMethods():this.Compilationinstead ofsemanticModel.Compilationwhen obtaining semantic models for callee syntax treesContainsSyntaxTree()checkthis.Compilationis the full compilation context containing all syntax trees being analyzed, whilesemanticModel.Compilationmay only contain a subset.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.